chore(reports): deprecate Slack v1 and harden Slack v2 tests#39914
Draft
sadpandajoe wants to merge 5 commits intomasterfrom
Draft
chore(reports): deprecate Slack v1 and harden Slack v2 tests#39914sadpandajoe wants to merge 5 commits intomasterfrom
sadpandajoe wants to merge 5 commits intomasterfrom
Conversation
…True, harden v2 tests Flips the ALERT_REPORT_SLACK_V2 feature flag default to True so the v2 auto-upgrade path runs out of the box, and adds one-shot DeprecationWarning + logger.warning emissions when v1 still runs (flag explicitly off, or bot missing the channels:read scope). Slack retired the legacy files.upload endpoint in 2025, so v1 file uploads are already broken at the API level — only text-only chat_postMessage sends still succeed via the legacy path. The bulk of the change is bulletproof unit-test coverage for SlackV2Notification ahead of v1 removal in the next major: - files_upload_v2 invocation with PNG (single + multiple), CSV, and PDF, asserting channel, file, title, filename, and initial_comment kwargs - multi-channel fan-out (3 channels x 2 files = 6 uploads) and text-only multi-channel chat_postMessage - inline-file precedence (CSV beats screenshots beats PDF) - parametrized exception mapping across 7 slack_sdk error types -> the 4 NotificationException subclasses - statsd .ok and .warning gauge emission via the @statsd_gauge decorator - execution_id propagation from g.logs_context to the success log, plus the falsy g.logs_context fallback path - end-to-end auto-upgrade round-trip: v1 SLACK recipient with channel names raises SlackV1NotificationError -> update_report_schedule_slack_v2 rewrites the row to channel IDs -> SlackV2Notification fast-paths the next send with no further channel resolution - should_use_v2_api() warning behavior: deprecation warning emitted exactly once across multiple calls in both the flag-off and scope-missing paths, with the scope-missing logger.warning continuing to fire each call so operators see the actionable scope hint in their report-execution logs Also locks in current behavior of the @backoff.on_exception(SlackApiError, ...) decorator on send(): because send() catches every SlackApiError internally and re-raises as NotificationUnprocessableException, backoff never sees the target exception type and no retries actually fire. Test asserts call_count == 1 with a docstring marking this as a known design issue to address separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39914 +/- ##
=======================================
Coverage 63.88% 63.89%
=======================================
Files 2583 2583
Lines 136602 136616 +14
Branches 31501 31502 +1
=======================================
+ Hits 87274 87292 +18
+ Misses 47812 47808 -4
Partials 1516 1516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…test With ALERT_REPORT_SLACK_V2 now defaulting to True, a SLACK recipient's first send triggers the v1->v2 auto-upgrade, which calls get_channels_with_search to resolve channel names to channel IDs. The existing test mocked WebClient.conversations_list to return a plain dict that lacked the `.data` attribute the upgrade path reads, so the upgrade raised "'dict' object has no attribute 'data'" and the test errored. Patch get_channels_with_search directly (matching the pattern already used by the other v2-conversion tests in this file) so the upgrade can resolve channels without going through the WebClient mock plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t errors The @backoff.on_exception decorator on SlackV2Notification.send() was configured to retry on SlackApiError, but the function's own try/except catches every SlackApiError and re-raises as NotificationUnprocessableException before the decorator can see it. As a result, no retries were happening — a single transient failure (rate limit, connection blip) would fail the report immediately, defeating the intent of the 5-attempt retry budget. Switch the decorator to retry on NotificationUnprocessableException, which is the exception type that send() actually raises for transient Slack failures (SlackApiError, SlackClientNotConnectedError, and the SlackClientError catch-all). Mirrors the working pattern already in webhook.py. Non-transient errors (NotificationParamException, NotificationMalformedException, NotificationAuthorizationException) still surface immediately — they aren't retryable and shouldn't be retried. Test changes: - Replaces the prior "locks in broken behavior" regression test with test_v2_send_retries_on_transient_slack_api_error asserting call_count == 5 - Adds test_v2_send_does_not_retry_param_errors verifying that BotUserAccessError → NotificationParamException is NOT retried (call_count == 1) - Adds an autouse fixture that patches backoff._sync.time.sleep so unit-test retries complete in milliseconds rather than the ~150s of real exponential backoff. Without this, the parametrized exception-mapping cases that map to NotificationUnprocessableException balloon the test runtime by ~75s The v1 SlackNotification has the same bug but is being deprecated in this release; not worth fixing there since v1's file_uploads endpoint is already dead at Slack's side and only the text-only chat_postMessage path still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sadpandajoe
added
the
risk:breaking-change
…-liner style Replaces the multi-section paragraph form with the single-bullet, PR-link-prefixed style used by the historical entries in this file (see the original Slack v2 deprecation in 4.1.0 / #29264). Same information, less ceremony. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-review changes: - Replace module-level `_v1_*_warning_emitted` booleans with `functools.cache`- decorated `_emit_v1_*_deprecation` helpers. Bare module globals had a read-then-write race under multi-threaded WSGI workers; functools.cache is thread-safe under the GIL and produces actually-once-per-process semantics without the noqa: PLW0603 escape hatch. - Mention `groups:read` (in addition to `channels:read`) wherever the scope requirement appears: deprecation message constant, config.py comment, the scope-missing logger.warning, UPDATING.md, and (auto-synced) feature-flags.json. The v2 channel resolver queries both public_channel and private_channel types, so granting only `channels:read` silently breaks private-channel reports. - Add `test_propagates_non_slack_api_errors_from_probe` — locks in that any exception other than SlackApiError (network, transport) propagates out of should_use_v2_api rather than masquerading as a missing-scope warning. - Drop a tautological `assert_not_called()` on `get_channels_with_search` in the auto-upgrade round-trip test. SlackV2Notification.send() never calls that helper in any path, so the assertion was true by construction rather than by the test exercising a real fast path. - Pin assertions on the deprecation-warning *message* to the exported `_SLACK_V1_DEPRECATION_MESSAGE` constant instead of substring fragments. - Update the test autouse fixture to clear the new functools.cache caches rather than reset the now-removed module globals. Three architectural concerns from review (auto-upgrade transaction race, concurrent worker upgrade race, end-of-deprecation cleanup migration) are pre-existing on the upgrade path and tracked as separate follow-up tasks rather than expanded into this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Deprecates the legacy Slack v1 integration for Alerts and Reports and adds bulletproof unit-test coverage for
SlackV2Notificationahead of v1 removal in the next major.Why now. Slack retired the
files.uploadendpoint in 2025, so v1 sends that include screenshots, CSVs, or PDFs already fail at the API level — only text-onlychat_postMessagesends still succeed via the legacy path. Existing recipients with thechannels:readscope already auto-upgrade toSlackV2on first send via theupdate_report_schedule_slack_v2flow; the only thing keeping the upgrade from running out of the box wasALERT_REPORT_SLACK_V2defaulting toFalse.What this change does:
ALERT_REPORT_SLACK_V2default toTrue(the docs JSON re-syncs fromconfig.pyautomatically via thefeature-flags-syncpre-commit hook).DeprecationWarning+logger.warningemissions when the v1 path still runs — both the flag-off and the missing-channels:readpaths. The scope-missinglogger.warningcontinues to fire every send so operators see the actionable scope hint in report-execution logs.# TODO: Remove in 6.0.0comment onSlackNotificationwith an accurate deprecation note (6.0 already shipped).## Nextwith operator action items (grantchannels:read, recipients auto-upgrade on next send, v1 removal targeted for the next major).Bulletproof v2 test coverage added in
tests/unit_tests/reports/notifications/slack_tests.pyandtests/unit_tests/utils/slack_test.py:files_upload_v2invocation with PNG (single + multiple), CSV, and PDF — assertingchannel,file,title,filename, andinitial_commentchat_postMessageslack_sdkerror types → the 4NotificationExceptionsubclasses.okand.warninggauge emission via the@statsd_gaugedecoratorexecution_idpropagation fromg.logs_contextto the success log, plus the falsy-fallback pathSLACKrecipient with channel names →SlackV1NotificationError→update_report_schedule_slack_v2rewrites the row to channel IDs → freshSlackV2Notificationfast-paths the next send with no resolver callshould_use_v2_api()warning behavior:DeprecationWarningemitted exactly once across multiple calls in both flag-off and scope-missing pathsReal bug surfaced (not fixed here):
SlackV2Notification.send()is decorated with@backoff.on_exception(SlackApiError, ..., max_tries=5), but the function catches everySlackApiErrorinternally and re-raises asNotificationUnprocessableExceptionbefore backoff can see it — so no retries actually fire. The testtest_v2_send_backoff_decorator_does_not_retry_swallowed_slack_api_errorslocks in current behavior (call_count == 1) with a docstring explaining the design issue. Worth a separate change to fix the retry config.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI changes.
TESTING INSTRUCTIONS
# Unit tests PYTHONPATH=/path/to/superset_core python3 -m pytest \ tests/unit_tests/reports/ \ tests/unit_tests/commands/report/ \ tests/unit_tests/utils/slack_test.pyAll 319 tests pass on this branch (29 in
slack_tests.py, 14 inutils/slack_test.py, plus the existing report unit suite).To validate the deprecation behavior manually:
ALERT_REPORT_SLACK_V2: Falseset explicitly insuperset_config.pyand a Slack recipient configured, fire a report. You should see a singleDeprecationWarningandlogger.warningline on the first send, then no more for that process.True) and a Slack bot lackingchannels:read, fire a report. You should seelogger.warningdescribing the missing scope on every send, plus a one-shotDeprecationWarning.channels:read, the first send for aSLACK-type recipient auto-upgrades the row toSLACKV2with channel IDs (existing behavior — now exercised by the round-trip unit test).ADDITIONAL INFORMATION
ALERT_REPORT_SLACK_V2default flipped toTrue🤖 Generated with Claude Code